Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

preflight: Ensure crc network is accessible from session libvirt #709

Closed
wants to merge 6 commits into from
Closed

preflight: Ensure crc network is accessible from session libvirt #709

wants to merge 6 commits into from
+194 −2

Conversation

zeenix
Copy link
Contributor

@zeenix zeenix commented Oct 11, 2019

This is needed to be able to switch to session libvirt, which will be through machine-driver-libvirt (See crc-org/machine-driver-libvirt#20).

@zeenix zeenix mentioned this pull request Oct 11, 2019
Closed
gbraad
gbraad reviewed Oct 11, 2019
View reviewed changes
pkg/crc/preflight/preflight_checks_linux.go Outdated
@@ -32,6 +32,8 @@ const (
crcNetworkManagerConfigFile = "crc-nm-dnsmasq.conf"
// This is defined in https://github.com/code-ready/machine-driver-libvirt/blob/master/go.mod#L5
minSupportedLibvirtVersion = "3.4.0"
qemuBridgeConfigPath1 = "/etc/qemu/bridge.conf"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the difference? One is Fedora, RHEL and the other is ... ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qemu is fedora, qemu-kvm is RHEL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

network or bridge?

Copy link
Contributor

@gbraad gbraad Oct 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/qemuBridgeConfigPath1/qemuBridgeConfigPathFedora
s/qemuBridgeConfigPath2/qemuBridgeConfigPathRHEL

or similar, as at the moment it is not clear why these paths are considered. And when distro specific checks are added, this becomes relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was also on Debian, @cfergeau ? Hence the numbering instead of distro names.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of an array/list but didn't go for that since there is only 2 paths

It is a better solution to use a list, and still use a named variables in the list. readability and maintainability are also key.

I hope that good example makes follow ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We inform the user WHAT we do and FIX the issue so their system MEETS PREREQUISITES.

but @cfergeau said it's NOT an issue but a distro decision hence it's not as simple as that for me.

It is a better solution to use a list, and still use a named variables in the list. readability and maintainability are also key.

I don't see how list is more readable with just two cases here but ok. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not actually addressed the setuid issue yet. According to this blog post, setuid is not set and /etc/qemu/bridge.conf need to be manually created. Do we want this addressed already in this PR or can it be a followup?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd first code this around Fedora/RHEL, so a followup is fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll resolve this once we merge the PR and I'll create an issue for this.

gbraad
gbraad requested changes Oct 11, 2019
View reviewed changes
Copy link
Contributor

@gbraad gbraad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear why path1 and path2 are used, and small function refactor needed to remove the append parameter as no default value parameters are possible.

@gbraad
Copy link
Contributor

gbraad commented Oct 11, 2019

Linting issues:

GOOS=linux /Users/distiller/go/bin/golangci-lint run
pkg/crc/preflight/preflight_checks_linux.go:403:3: printf: Debug call has possible formatting directive %s (govet)
		logging.Debug("Failed to open %s: %s, trying %s..", qemuBridgeConfigPath1, err, qemuBridgeConfigPath2)
		^
pkg/crc/preflight/preflight_checks_linux.go:426:3: printf: Debug call has possible formatting directive %s (govet)
		logging.Debug("Failed to open %s: %s, trying %s..", qemuBridgeConfigPath1, err, qemuBridgeConfigPath2)
		^

@cfergeau
Copy link
Contributor

One note about terminology, you are talking about 'crc network' in this pull request, which for me means 'crc libvirt network', while what the preflight checks really are doing is allowing the crc bridge (as seen in 'ip addr' for example) to be used by qemu:///session

@gbraad
Copy link
Contributor

gbraad commented Oct 11, 2019

@cfergeau this might be due to us, but what would be 'easier' to understand to the user?

@zeenix
Copy link
Contributor Author

zeenix commented Oct 14, 2019 •
edited
Loading

One note about terminology, you are talking about 'crc network' in this pull request, which for me means 'crc libvirt network', while what the preflight checks really are doing is allowing the crc bridge (as seen in 'ip addr' for example) to be used by qemu:///session

As @gbraad said, it might be better to not confuse the user with too precise terminology here.

@cfergeau
Copy link
Contributor

One note about terminology, you are talking about 'crc network' in this pull request, which for me means 'crc libvirt network', while what the preflight checks really are doing is allowing the crc bridge (as seen in 'ip addr' for example) to be used by qemu:///session

As @gbraad said, it might be better to not confuse the user with too prices terminology here.

There are several different things, in user-visible strings, we can discuss whether to use "crc network", "crc network device", "crc bridge").
In function names and debug messages, the correct terminology should be used.

@zeenix
Copy link
Contributor Author

zeenix commented Oct 14, 2019

PTALA.

gbraad
gbraad requested changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@gbraad gbraad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prevent nested-if statements.

@zeenix
Copy link
Contributor Author

zeenix commented Oct 16, 2019

PTALA

gbraad
gbraad requested changes Oct 23, 2019
View reviewed changes
Copy link
Contributor

@gbraad gbraad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of duplication remains

@zeenix zeenix force-pushed the session branch 3 times, most recently from dfe0438 to df61101 Compare October 25, 2019 11:19
@praveenkumar
Copy link
Member

@zeenix looks like all the test are failing may be you need to check make fmt and make lint.

@zeenix
Copy link
Contributor Author

zeenix commented Jan 16, 2020

@zeenix looks like all the test are failing may be you need to check make fmt and make lint.

@praveenkumar I first need to make the latest changes work. :) I think i've fixed that on crc side and now looking at the machine driver side..

zeenix added 6 commits January 16, 2020 15:01
@zeenix
util_linux: Add AppendToFileAsRoot utility function
0b6970e 
Add a utility function similar to WriteToFileAsRoot(), that appends to the
specified path instead of overwriting the file. We use this in a following
patch to append to Qemu bridge access configuration file.
@zeenix
Add GetFirstExistentPath() utility function
1c6852c 
Given a set of paths, it retrieves the first existent path.
@zeenix
preflight: Ensure crc network is accessible from session libvirt
294f4bc 
This is needed to be able to switch to session libvirt, which will be
through machine-driver-libvirt (See
crc-org/machine-driver-libvirt#20)
@zeenix
libvirt: Ensure bridge name is the same as network name
0a3bf42 
This is to ensure that if we decided to change the network name in the
future, the developer doesn't forget to change the bridge name as well.
@zeenix
Add utility function for chown
e958abe
@zeenix
preflight: Import existing VM from system libvirt connection
ae0ba7d 
This includes:

* Deleting the `crc` VM from system libvirt.
* Ensuring the disk image has the current user as owner.
@stale
Copy link

stale bot commented Mar 16, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status/stale Issue went stale; did not receive attention or no reply from the OP label Mar 16, 2020
@anjannath anjannath removed the status/stale Issue went stale; did not receive attention or no reply from the OP label Mar 17, 2020
@stale
Copy link

stale bot commented May 15, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status/stale Issue went stale; did not receive attention or no reply from the OP label May 15, 2020
@stale stale bot closed this May 29, 2020
@cfergeau cfergeau removed the status/stale Issue went stale; did not receive attention or no reply from the OP label Jun 2, 2020
@cfergeau cfergeau reopened this Jun 2, 2020
@stale
Copy link

stale bot commented Aug 1, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status/stale Issue went stale; did not receive attention or no reply from the OP label Aug 1, 2020
@stale stale bot closed this Aug 15, 2020
@cfergeau cfergeau reopened this Aug 21, 2020
@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@stale stale bot removed the status/stale Issue went stale; did not receive attention or no reply from the OP label Aug 21, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zeenix
To complete the pull request process, please assign gbraad
You can assign the PR to them by writing /assign @gbraad in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot
Copy link

@zeenix: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@cfergeau cfergeau added the status/pinned Prevents the stale bot from closing the issue label Aug 21, 2020
@guillaumerose
Copy link
Contributor

We will reopen this PR if needed. crc-org/machine-driver-libvirt#20 is still opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfergeau cfergeau cfergeau requested changes

@gbraad gbraad gbraad requested changes

@praveenkumar praveenkumar Awaiting requested review from praveenkumar

Assignees
No one assigned
Labels
needs-rebase status/pinned Prevents the stale bot from closing the issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@zeenix @gbraad @cfergeau @praveenkumar @centos-ci @openshift-ci-robot @guillaumerose @anjannath